[refactor](be) Unify storage reader column ordinals - #66472
Open
csun5285 wants to merge 5 commits into
Open
Conversation
csun5285
requested review from
924060929,
airborne12,
deardeng,
eldenmoon,
englefly,
gavinchou,
liaoxin01,
luwei16,
morrySnow,
starocean999 and
yiguolei
as code owners
August 5, 2026 06:26
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
csun5285
force-pushed
the
refactor/reader-return-columns
branch
2 times, most recently
from
August 7, 2026 12:27
6a3b439 to
768a009
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29131 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-DS: Total hot run time: 158453 ms |
Contributor
ClickBench: Total hot run time: 23.85 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
Aug 9, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#66472 Problem Summary: The reader-schema refactor renamed storage block and schema APIs, but several BE tests still used the old interfaces. The FE translator test also left row-binlog index replicas behind the partition version, and one DATETIMEV2 fixture omitted its scale. Update these test fixtures so the refactored reader code is compiled and exercised correctly. ### Release note None ### Check List (For Author) - Test: Unit Test and build - ASAN BE and FE build - PhysicalPlanTranslatorTest: 13 tests passed - Reader-related BE UT: 71 tests passed - clang-format and clang-tidy passed - Behavior changed: No - Does this need documentation: No
csun5285
force-pushed
the
refactor/reader-return-columns
branch
from
August 9, 2026 14:20
768a009 to
aff0dbe
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes for two P1 correctness/compatibility blockers in the new FE-authored ReadSchema contract.
Critical checkpoints:
- Goal and tests: The ordinal unification is coherent on most current-FE paths, but the added tests primarily assert descriptor shape; they do not prove lazy scan outcomes or mixed-version execution.
- Scope: Reviewed the complete 124-file authoritative diff and the upstream/downstream FE scan, BE reader, merge, index, schema-change, compaction, cloud, binlog, snapshot, rowid, and test paths.
- Concurrency: No new race was substantiated; shared read-schema mutation is copy-before-publish and occurs before rowset-reader context capture.
- Lifecycle: Delete-predicate suffix construction, reader initialization, segment state sizing, and visible-prefix output boundaries are coherent.
- Configuration: The lazy bug is reachable under the existing TopN lazy-materialization threshold; no separate configuration/default regression was found.
- Compatibility: Blocking issue inline: an older FE can send the same accepted execution version while omitting dependencies that this BE now assumes are present.
- Parallel paths: Query/direct readers, vertical readers, schema change, compaction, cloud, inverted/ANN, and rowid paths were traced; the distinct missed parallel path is lazy scan translation.
- Conditions: Key/value predicate ordinal binding, TSO predicate creation, condition-cache fencing, and historical delete conditions were checked; no additional issue survived.
- Test coverage/results: No local build or tests were run, as required by the review environment. Visible CI has successful BE UT (macOS), CheckStyle, Clang Formatter, license, secrets, and dependency checks; other displayed jobs are skipped, and this automated review status is pending until submission.
- Observability: The ReadSchema profile output is useful and no logging/profile correctness issue was found.
- Persistence/transactions: The runtime-only schema object does not alter persisted metadata; rowset versions, delete history, snapshot fences, and transaction-facing row images were reviewed.
- Data writes: Writer, memtable, compaction, and rowid-conversion edits are mechanical ordinal/schema migrations with no substantiated write-path defect.
- FE/BE variables:
be_exec_version=11does not encode the new tuple-completeness capability, which is the compatibility blocker noted inline. - Performance: No clear performance regression was found in the ordinal maps, column state, or reader construction changes.
- Other issues: Blocking issue inline: lazy translation can remove keys and snapshot/binlog helper slots before BE constructs ReadSchema.
User focus: review_focus.txt contained no additional focus beyond the full PR review.
Review completion: The full-scope and risk-focused passes converged in Round 2 with no unresolved candidates.
| .collect(Collectors.toSet()); | ||
|
|
||
| olapScanNode.updateRequiredSlots(context, scanIds); | ||
| preserveMergeSequenceSlots(olapScanNode, scanIds); |
Contributor
There was a problem hiding this comment.
[P1] Preserve the full storage dependency set for lazy scans
This second pruning pass bypasses the normal calls to preserveStorageSemanticSlots() and preserveExtraStorageKeySlots(). For the new test query, k1/k2 are lazy, so the scan descriptor keeps v1/sequence/rowid but drops the key prefix; BE then builds a ReadSchema with zero key columns and can merge distinct MOR keys as equal. Lazy snapshot/binlog scans similarly lose commit-TSO or TSO/op/before slots and fail initialization. Apply the same two preservation steps used by updateScanSlotsMaterialization before removing tuple slots.
| return Status::InternalError("failed to build storage scanner, no materialized slot!"); | ||
| } | ||
|
|
||
| // The FE physical scan tuple is the read-path schema. It already includes |
Contributor
There was a problem hiding this comment.
[P1] Keep a fallback for plans from older FEs
This assumes every accepted FE plan already carries the new hidden dependencies, but the PR removes the BE-side expansion that older FEs rely on and adds no thrift/version handshake. The immediately prior FE emits the same be_exec_version=11, so its ordinary plans can reach this BE without sequence/map, commit-TSO, or row-binlog helper slots; the BE then silently chooses the wrong MOR winner or fails _init_tso_predicates/init_sequence_map. Retain legacy dependency expansion (with ordinal remapping), or add an explicit plan capability marker before treating the tuple as authoritative.
csun5285
changed the title
Contributor
|
run buildall |
yiguolei
reviewed
Aug 12, 2026
| // merge or union segment iterator | ||
| if (is_merge_iterator()) { | ||
| auto sequence_loc = -1; | ||
| if (_read_context->sequence_id_idx != -1) { |
Contributor
There was a problem hiding this comment.
sequence_id_idx 这个参数之前是做什么的？
Contributor
Author
There was a problem hiding this comment.
如果 sequence_id_idx 是-1，表示不需要根据seq 列排序，如果不是 -1 ，就表示这一列的位置，就找到seq 列，后面排序会用，现在用 use_sequence_column_for_merge_order 表示是不是需要根据 seq 排序。
yiguolei
reviewed
Aug 12, 2026
| return Status::RuntimeError("invalid data type"); | ||
| } | ||
| auto column = data_type->create_column(); | ||
| *block = _read_schema->create_read_block(); |
Contributor
There was a problem hiding this comment.
我感觉，我们可能确实得在read schema 中，对于struct 列裁剪这种，我们得把它的column 结构改掉。 不能依靠传递一个data type。
否则很多地方如果有人去把tablet column 拿出来，做一些事情，比如创建一个column 似乎结构就不对了
yiguolei
reviewed
Aug 12, 2026
| auto column = data_type->create_column(); | ||
| *block = _read_schema->create_read_block(); | ||
| auto columns_guard = block->mutate_columns_scoped(); | ||
| for (auto& column : columns_guard.mutable_columns()) { |
Contributor
There was a problem hiding this comment.
我们之前为什么不是在segment iteartor 中把新加的delete dropped column 删掉啊，为啥要传递会这些iterator 中呢
Contributor
Author
There was a problem hiding this comment.
就是在segment iteartor 中删掉的，这里是在重置block
Contributor
|
/review |
yiguolei
reviewed
Aug 12, 2026
| auto storage_format = _opts.tablet_schema->get_inverted_index_storage_format(); | ||
| for (int i = 0; i < _schema->columns().size(); ++i) { | ||
| for (size_t i = 0; i < _schema->num_read_columns(); ++i) { | ||
| const TabletColumn* col = _schema->column(i); |
Contributor
There was a problem hiding this comment.
在light weight schema change 加列的时候，这个col 实际在当前segment 中不存在，我们怎么处理呢？
Contributor
Author
There was a problem hiding this comment.
new_column_iteator 会返回一个default_column_iterator
Contributor
There was a problem hiding this comment.
Automated review completed for 495247871d4fd4089ca750c5e0a7d053dd56a3c8 with no new actionable, non-duplicate findings.
Critical checkpoint conclusions:
- Goal, scope, and tests: The ReadSchema refactor consistently makes the caller-visible scan tuple the storage read-coordinate prefix, with historical delete columns kept as a private suffix. All 144 authoritative changed paths were reviewed. The added BE/FE tests and regression suites exercise nested pruning, historical delete columns, row-binlog projections, sequence-map dependencies, and lazy materialization. Per the review-runner instructions, I did not build or run tests.
- Concurrency and lifecycle: Per-reader ReadSchema mutation is isolated where sequence maps are initialized, dropped-column descriptors are appended before iterator construction, and cloned LIKE predicates own separate search scratch state. No new shared-state, lifetime, or static-initialization issue was found.
- Configuration and compatibility: Condition-cache and statistics fast paths are disabled where TSO/delete semantics require row evaluation; TopN lazy materialization is fenced for sequence-map tables. Current-FE/current-BE dependency population is coherent. The old-FE/new-BE dependency gap is already covered by existing review comment 3745935085, so it is not duplicated here; the earlier lazy second-pruning issue in 3745935083 is fixed at this head.
- Parallel and special paths: Ordinary and lazy scans, selected indexes, local/cloud schema change, horizontal/vertical/segment compaction, sequence and sequence-map merge, APPEND_ONLY/MIN_DELTA/DETAIL binlog reads, snapshot commit TSO, nested complex pruning, ANN/inverted/LIKE predicates, and Variant paths were traced through their consumers. No distinct correctness gap remained.
- Persistence, transactions, and writes: Historical delete predicates retain UID-based generation identity, binlog BEFORE/AFTER and TSO ordering remain aligned, and compaction/schema-change writers receive only the visible ReadSchema block prefix. The patch adds no new persisted configuration or protocol field.
- FE/BE contract, performance, and observability: FE preserves all current storage-semantic slots before final pruning, and BE consumes the same tuple ordinals. The remaining descriptor-based sequence check can only reduce predicate-pushdown reachability; it does not change results. Updated counters and failure paths remain adequate for diagnosis.
User focus: no additional review focus was supplied. The full PR was reviewed without extra narrowing.
yiguolei
reviewed
Aug 12, 2026
| std::vector<ColumnId> _common_expr_ordinals; | ||
| std::vector<ColumnId> _output_ordinals; | ||
| // Sparse, ordered execution list for recovering pruned nested children. | ||
| std::vector<ColumnId> _lazy_pruned_ordinals; |
yiguolei
reviewed
Aug 12, 2026
| key_range.lower_key != nullptr ? key_range.lower_key->schema() : nullptr; | ||
| if (key_range.upper_key != nullptr && | ||
| (key_schema == nullptr || | ||
| key_range.upper_key->schema()->num_read_columns() > key_schema->num_read_columns())) { |
Contributor
There was a problem hiding this comment.
它之前是两个key schema的union，但是新的代码，似乎认为是upper 的key schema 会包含lower？感觉这两者之间没什么必然关系
Contributor
Author
There was a problem hiding this comment.
两个key schema 长的一样因为
yiguolei
reviewed
Aug 12, 2026
| key_schema = key_range.upper_key->schema(); | ||
| } | ||
| if (key_schema == nullptr) { | ||
| return Status::OK(); | ||
| } | ||
| if (!_seek_schema) { |
Contributor
There was a problem hiding this comment.
这个代码也很诡异，难道多个key range的schema 都一样？
yiguolei
reviewed
Aug 12, 2026
| _seek_block[i] = Schema::get_data_type_ptr(*column_desc)->create_column(); | ||
| i++; | ||
| if (_seek_block.empty()) { | ||
| _seek_block.resize(_seek_schema->num_read_columns()); |
Contributor
There was a problem hiding this comment.
这里跟直接调用seek schema create read block 什么区别？
yiguolei
reviewed
Aug 12, 2026
| key_schema = key_range.upper_key->schema(); | ||
| } | ||
| if (key_schema == nullptr) { | ||
| return Status::OK(); | ||
| } | ||
| if (!_seek_schema) { |
Contributor
There was a problem hiding this comment.
如果这里认为肯定相等，我们就得加一些检查，防止我们这种假定可能不对，包括之前的lower key的schema和upper key的schema的问题
yiguolei
reviewed
Aug 12, 2026
| } | ||
| const TabletColumn* col = _seek_schema->column(i); | ||
| int32_t read_ordinal = _schema->ordinal_by_column(*col); | ||
| if (read_ordinal >= 0 && _column_iterators[read_ordinal] != nullptr) { |
yiguolei
reviewed
Aug 12, 2026
| } | ||
| std::unique_ptr<ColumnIterator> iter; | ||
| RETURN_IF_ERROR( | ||
| _segment->new_column_iterator(*col, &iter, &_opts, &_variant_sparse_column_cache)); |
Contributor
There was a problem hiding this comment.
为什么没有吧新的iter 放到 _column_iterators 里？
yiguolei
reviewed
Aug 12, 2026
| const TabletColumn* col = _seek_schema->column(i); | ||
| int32_t read_ordinal = _schema->ordinal_by_column(*col); | ||
| if (read_ordinal >= 0 && _column_iterators[read_ordinal] != nullptr) { | ||
| _seek_column_iterators[i] = _column_iterators[read_ordinal].get(); |
Contributor
There was a problem hiding this comment.
什么时候会出现，这个column iterator 只在seek 里，但是不在_column_iterators 里？
Contributor
Author
There was a problem hiding this comment.
比如这个key 列查询的时候没有读，这时候就只在seek 里
yiguolei
reviewed
Aug 12, 2026
| bool result_true = _check_all_conditions_passed_inverted_index_for_column(cid); | ||
| if (result_true) { | ||
| _need_read_data_indices[cid] = false; | ||
| _column_states[cid].need_read_data = false; |
yiguolei
reviewed
Aug 12, 2026
| @@ -1054,7 +1031,7 @@ Status SegmentIterator::_apply_ann_topn_predicate() { | |||
| // reference count of result_column should be 1, so move will not issue any data copy. | |||
| virtual_column_iter->prepare_materialization(std::move(result_column), result_row_ids); | |||
|
|
|||
| _need_read_data_indices[src_cid] = false; | |||
| _column_states[src_cid].need_read_data = false; | |||
Contributor
Author
There was a problem hiding this comment.
这里只是名字的替换，之前保存在_need_read_data_indices 中，现在都保存在 column_state. need_read_data 中
yiguolei
reviewed
Aug 12, 2026
| @@ -1467,38 +1402,31 @@ bool SegmentIterator::_need_read_data(ColumnId cid) { | |||
| // A column can skip data reads when its predicates have already been fully resolved. | |||
| // zonemap_always_true_pred_cols is produced only for non-key columns because key columns | |||
| // must remain readable for short-key range seeks. | |||
| const bool used_by_common_expr = | |||
| cid < _is_common_expr_column.size() && _is_common_expr_column[cid]; | |||
| const bool used_by_common_expr = _column_states[cid].is_common_expr; | |||
Contributor
There was a problem hiding this comment.
如果一个 where col1 > 1 and abs(col1) < 100
此时这个col1 既是 predicate 也是common expr 你这个标记怎么办？
yiguolei
reviewed
Aug 12, 2026
| if (_cur_rowid >= num_rows()) { | ||
| return Status::OK(); | ||
| } | ||
|
|
||
| for (auto cid : _schema->column_ids()) { | ||
| for (uint32_t cid = 0; cid < _schema->num_read_columns(); ++cid) { | ||
| if (!_is_active_read_column(cid)) { |
Contributor
There was a problem hiding this comment.
换成 _column_states[cid].is_predicate() 这个吧
yiguolei
reviewed
Aug 12, 2026
| @@ -1791,7 +1710,7 @@ Status SegmentIterator::_lookup_ordinal_from_sk_index(const RowCursor& key, bool | |||
| while (start < end) { | |||
| rowid_t mid = (start + end) / 2; | |||
| RETURN_IF_ERROR(_seek_and_peek(mid)); | |||
| int cmp = _compare_short_key_with_seek_block(key, key_col_ids); | |||
Contributor
Author
There was a problem hiding this comment.
这里指的是 upper key 里面带的所有的key
yiguolei
reviewed
Aug 12, 2026
| _short_cir_eval_predicate.push_back(predicate); | ||
| } | ||
| if (predicate->is_runtime_filter()) { | ||
| _filter_info_id.push_back(predicate); |
yiguolei
reviewed
Aug 12, 2026
| auto cid = _schema->column_id(i); | ||
| const auto* column_desc = _schema->column(cid); | ||
| for (ColumnId i = 0; i < _schema->num_read_columns(); i++) { | ||
| if (!_is_active_read_column(i)) { |
Contributor
Author
There was a problem hiding this comment.
drop column，并且这个segment 不需要执行 delete pred
yiguolei
reviewed
Aug 12, 2026
| current_columns[cid] = std::move(*block->get_by_position(i).column).mutate(); | ||
| current_columns[cid]->reserve(nrows_read_limit); | ||
| } | ||
| current_columns[i] = std::move(*block->get_by_position(i).column).mutate(); |
Contributor
There was a problem hiding this comment.
这里check 一下，这个block 里的column 的类型，和 它必须是empty
yiguolei
reviewed
Aug 12, 2026
| const auto& file_column_type = _storage_name_and_type[i].second; | ||
| const auto& expected_type = _schema->data_type(i); | ||
| if (_column_states[i].is_predicate()) { | ||
| if (current_columns[i].get() == nullptr) { |
Contributor
There was a problem hiding this comment.
predicate column 中有2类：
- 单纯是delete 的
- 还有用户输入的column
我们不需要跟要返回的block 关联吗？
yiguolei
reviewed
Aug 12, 2026
| // Ordered, disjoint column-role lists built once and reused by every batch. | ||
| std::vector<ColumnId> _predicate_ordinals; | ||
| std::vector<ColumnId> _common_expr_ordinals; | ||
| std::vector<ColumnId> _output_ordinals; |
Contributor
There was a problem hiding this comment.
这里你得举个例子，把这几种ordinal 都解释一下
yiguolei
reviewed
Aug 12, 2026
| cid, column_in_block_is_nothing, column_is_normal, current_column_is_nothing); | ||
|
|
||
| if (column_in_block_is_nothing || column_is_normal) { | ||
| block->replace_by_position(cid, std::move(_current_columns[cid])); |
Contributor
There was a problem hiding this comment.
按照 current_columns[i] = std::move(*block->get_by_position(i).column).mutate(); 这个写法，它不是本来就是在block 里吗？为啥这里还要replace 一下？
Contributor
Author
There was a problem hiding this comment.
看起来这里不用 replace，难道是 column_in_block_is_nothing 的时候需要特别处理？
yiguolei
reviewed
Aug 12, 2026
| @@ -2929,10 +2707,10 @@ Status SegmentIterator::_next_batch_internal(Block* block) { | |||
| return _process_eof(block); | |||
| } | |||
|
|
|||
| if (_is_need_vec_eval || _is_need_short_eval || _is_need_expr_eval) { | |||
| if (has_residual_eval) { | |||
yiguolei
reviewed
Aug 12, 2026
| _sel_rowid_idx.data(), _selected_size)); | ||
|
|
||
| // step 3.2: read remaining expr column and evaluate it. | ||
| if (_is_need_expr_eval) { | ||
| // The predicate column contains the remaining expr column, no need second read. | ||
| if (_common_expr_column_ids.size() > 0) { | ||
| if (!_common_expr_ordinals.empty()) { | ||
| SCOPED_RAW_TIMER(&_opts.stats->non_predicate_read_ns); | ||
| RETURN_IF_ERROR(_read_columns_by_rowids( |
Contributor
There was a problem hiding this comment.
如果一列，既是 predicate ordinal，又是 expr ordinal 怎么办？
Contributor
Author
There was a problem hiding this comment.
已经在前面 pred 的时候读过，放到 block 里面了，这里的 _common_expr_ordinals 是不包含的
yiguolei
reviewed
Aug 12, 2026
| RETURN_IF_ERROR(_convert_to_expected_type(ordinals)); | ||
| for (auto cid : ordinals) { | ||
| DCHECK_LT(cid, _schema->num_block_columns()); | ||
| block->replace_by_position(cid, std::move(_current_columns[cid])); |
Contributor
There was a problem hiding this comment.
我们有很多个这个replace 都是啥含义啊，感觉应该是最后返回的时候replace 一次就行
yiguolei
reviewed
Aug 12, 2026
| } | ||
| for (size_t i = 0; i < block->columns(); i++) { | ||
| if (!_column_states[i].is_predicate()) { | ||
| block->replace_by_position(i, std::move(_current_columns[i])); |
Contributor
There was a problem hiding this comment.
这又来了一波replace，包括之前那个convert，我感觉都是一个事，表达了，当我们要把current columns 的结果，放进block 里这个事
yiguolei
reviewed
Aug 12, 2026
| std::set<std::shared_ptr<const ColumnPredicate>> delete_predicates; | ||
| _opts.delete_condition_predicates->get_all_column_predicate(delete_predicates); | ||
| for (const auto& predicate : delete_predicates) { | ||
| _convert_dict_code_for_predicate_if_necessary_impl(*predicate); |
Contributor
There was a problem hiding this comment.
_col_predicates 里不包括 delete 吗
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)